Fix guess_TopologyAttrs for empty Elements and Atomtypes#5416
Fix guess_TopologyAttrs for empty Elements and Atomtypes#5416AnimeshPriyanshu wants to merge 1 commit into
Conversation
|
2ee98ed to
4c1cd4b
Compare
There was a problem hiding this comment.
Have you looked deeper into the guessers? Is there perhaps a reason why Elements does not have a value?
There's another failing test test_wrong_elements_warnings whose outcome your patch changes. You need to understand if that should or should not happen.
You are also renaming and changing an existing test. You should not be doing this lightly as these tests may check expected behavior. At a minimum you have to explain why you are making these changes.
I understand that the poster on issue #5414 suggested the minimal fix but to be honest, this looked like a LLM-suggestion to me. That doesn't mean it's necessarily wrong but it may not have the full picture.
Basically, can you explain in your own words why your fix is the right solution to the problem?
| atol=0, | ||
| ) | ||
|
|
||
| def test_partial_guess_attr_with_unknown_no_value_label(self): |
There was a problem hiding this comment.
The fact that there's a test that checks for the behavior that you're changing should at least give pause for thought.
Why are you changing the name and the outcome for this test?
| assert_equal(u.atoms.types, ["", "", "", ""]) | ||
| assert_equal(u.atoms.types, ["C", "H", "H", "O"]) | ||
|
|
||
| def test_partial_guess_elements_with_missing_value_label(self): |
There was a problem hiding this comment.
Adding this test that checks for your functionality is ok.
There was a problem hiding this comment.
There are a lot of formatting changes in the file that make the diff hard to read. Why did you change the formatting?
|
Thank you @orbeckst for the detailed feedback. I really appreciate it. This is my first open-source contribution, so I initially focused on the reasoning described in #5414 and implementing the proposed fix. After reading your comments, I realize I need to understand the existing design more deeply instead of assuming the proposed solution is the correct one. I'll investigate the full guessing flow (GuesserBase, TopologyAttr.are_values_missing, DefaultGuesser, and the PDB parser) to understand whether the absence of missing_value_label for Elements and Atomtypes was intentional or an oversight. I'll also look into why test_wrong_elements_warnings changes and whether updating that test is actually the correct behavior, rather than simply changing it to make the tests pass. I'll update the PR once I've investigated it thoroughly. |
|
Hi guys, recently I started to analyze this project and understaning the issues as a new contributor. I wanted to understand if my reasoning about this Pull request is correct. so what happens is that while building an Universe, missing element values get stored as "", and we are able to use guess_TopologyAttrs to fill these attributes. What happens is that this topology guessing function recognizes which atoms need this treatement by asking the Elements class which value should be treated as missing. However, it looks for missing_value_label, which does not currently exist in the Elements class, and ends up using the fallback value None, which is never going to match with the "" that was stored. So what we have to do is define this label to make the guessing function recognize those values as missing. About the changing of the test, that test had exactly the scope of proving that even in the case of empty strings the guesser was not going to start without being able to find missing_value_label. Now this test does not have the same meaning anymore, because Atomtypes would no longer represent a class without a missing_value_label. |
3 participants
Fixes #5414
Changes made in this Pull Request
missing_value_label = ""toElementsandAtomtypesso empty string values are recognized as missing during topology attribute guessing.Atomtypespartial guessing test to reflect the new behavior.LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.